Feature/1797#123
Conversation
| @@ -0,0 +1,6 @@ | |||
| name: uhadoop | |||
| owners: | |||
| - mark.wei@ucloud.cn | |||
Episkey-G
left a comment
Episkey-G
left a comment
There was a problem hiding this comment.
评审结论:Request changes
按《UCloud CLI 产品命令开发与接入规范》逐项核对,并把 PR 分支拉到本地实际跑了平台闸门。目录结构与大部分写法符合规范,机器闸门全绿,但有 2 类阻塞问题(功能缺陷 + 输出契约)与 owners 配置错误需要先解决。
已验证通过 ✅
| 检查 | 结果 |
|---|---|
go build ./... |
通过 |
go run ./hack/check-product(规则 1-10) |
全部通过 |
go test ./hack/snapshot -run 'TestProductGoldens/uhadoop|TestProductCompletionGoldens/uhadoop' |
通过,golden 与命令树一致 |
目录布局、一 verb 一文件命名(add_node.go→add-node)、cli.NewServiceClient、危险操作 ctx.Confirm+--yes、长耗时 --async+Spoll、products.gen.go 由 gen-products 正确再生成——这些都符合规范,基础打得不错。👍
阻塞项 🔴
- owners 必须改为 GitHub 用户名
markwei-ucloud(product.yaml 行级评论已提,这里补充机制依据):owner-gate 用github.event.pull_request.user.login与 owners 逐项EqualFold比较(hack/owner-gate/main.go:201+.github/workflows/pr-gate.yml:134),邮箱永远匹配不上。不改的话合并后本产品所有后续 PR 都会被判 non-owner 转平台审批,owner 自治与 auto-merge 对 uhadoop 永久失效。现有产品均为 GitHub 用户名(如 ufs 的pearlinpan)。另请补首行注释,与其他产品 yaml 保持一致:# products/uhadoop/product.yaml — uhadoop 产品元数据(归属真源,owner 自治维护)。 list强制--zone,--all-region整体不可用——实测复现,TEST.md 自己的示例命令现状直接报错,详见 list.go 行级评论。- delete / restart-service 违反输出契约(规范 §2.5/§6/§11 禁止项)——
PrintJSON/ctx.Err()应改为ctx.EmitResult/ctx.ProgressWriter(),详见行级评论。
建议项 🟡
- create/add-node/upgrade-node/upgrade-disk 手写公共 flag,丢失 region/zone/project-id 补全,统一改
ctx.BindCommonParams(§8); - 全部 10 个子命令
MarkFlagRequired("region")/("zone"),profile 默认失效且与 "Optional." 描述自相矛盾,读类命令应去掉(§7); - describe.go 的无效
SetFlagValues(region)删除;concern 归位:StateRunning→status.go、describeClusterForPoll→poll.go、sdkStr→用sdk.String(§2.2/§2.3);rows.go 的frameworkAppRow死代码; - upgrade-* 的 node-role 补全含
client与描述矛盾; - TEST.md 清理个人环境路径;测试结果表 14 项全部为 ⬜ 待测试,请真实跑完勾选后再合——这是本 PR 的验收证据;
- create/list/describe/list-framework-app 四处绕开 SDK 类型走
InvokeAction的 workaround(SDK 类型与 API 不符,注释已说明)可以接受,但请在 PR 描述显式声明,并给 ucloud-sdk-go 提修复 issue,SDK 修复后移除(§10 精神)。
提醒:凡涉及 flag required 状态、默认值或补全候选的调整,改完记得按 §2.4 重刷本产品两份 golden(
WRITE_CMDTREE_GOLDEN=1/WRITE_COMPLETION_GOLDEN=1),随同一 PR 提交。
| command.SetFlagValues(cmd, "id-only", "true", "false") | ||
|
|
||
| cmd.MarkFlagRequired("region") | ||
| cmd.MarkFlagRequired("zone") |
There was a problem hiding this comment.
🔴 阻塞:list 强制 --zone 后基本不可用,本 PR 的 TEST.md §1.3 示例即可复现:
$ ucloud uhadoop list --region cn-bj2
Error: required flag(s) "zone" not set
ListUHadoopInstance 按 region 列集群本不需要 zone;全仓也没有其他产品的 list 把 zone 设为 required(对照 uhost list:region/zone 均 Optional 且走 profile 默认)。请去掉 MarkFlagRequired("zone")(region 同理,见 create.go 处的整体评论),并重刷 cmdtree.golden。
| return nil, err | ||
| } | ||
| for _, region := range regions { | ||
| _req := *req |
There was a problem hiding this comment.
🔴 阻塞:--all-region 模式整体不可用,两个原因:
- region/zone 被标 required,
uhadoop list --all-region直接报required flag(s) "region", "zone" not set(实测); - 即使凑齐参数,这里
_req := *req浅拷贝保留了用户传入的 Zone 指针,fan-out 到每个 region 的请求都带着同一个 zone(拿着 cn-bj2-02 去查上海),其他地域基本查不出结果。
fan-out 前应把 _req.Zone 置空(或复制时显式清掉),并解除 all-region 与 required region/zone 的冲突。
| return fmt.Errorf("[%d] %s", resp.RetCode, resp.Message) | ||
| } | ||
| fmt.Fprintf(ctx.Err(), "Cluster %s deleted\n", id) | ||
| ctx.PrintJSON(resp) |
There was a problem hiding this comment.
🟠 输出契约违规(规范 §2.5/§6/§11 禁止项):写命令的结构化结果必须走 ctx.EmitResult(cli.OpResultRow{...}),人读文案走 ctx.ProgressWriter()。当前 fmt.Fprintf(ctx.Err(), ...) + ctx.PrintJSON(resp) 的问题:table 模式下也会把原始 SDK 响应整坨 JSON 打到 stdout;--output json 时结构又与其他命令的 OpResultRow 不一致,脚本无法统一解析。
同时建议与本产品其余 8 个命令保持一致,改回 Run + ctx.HandleError(RunE 返回裸 error 绕开了平台统一错误上报格式)。参考本 PR 内 create/add-node 的收尾写法,或 examples/product_onboarding/delete.go。
| return fmt.Errorf("[%d] %s", resp.RetCode, resp.Message) | ||
| } | ||
| fmt.Fprintf(ctx.Err(), "Service %s %s on cluster %s, state: %s\n", *req.ServiceName, action, *req.InstanceId, resp.State) | ||
| ctx.PrintJSON(resp) |
There was a problem hiding this comment.
🟠 同 delete.go 的输出契约问题:去掉 ctx.PrintJSON(resp),人读文案改走 ctx.ProgressWriter(),收尾 ctx.EmitResult(cli.OpResultRow{ResourceID: *req.InstanceId, Action: action, Status: resp.State}),错误处理改 Run + ctx.HandleError。
| ctx.BindRegion(cmd, req) | ||
| ctx.BindZone(cmd, req) | ||
| ctx.BindProjectID(cmd, req) | ||
| req.InstanceId = cmd.Flags().String("instance-id", "", "Required. Cluster instance ID") |
There was a problem hiding this comment.
🟡 描述写 "Required." 但没有 cmd.MarkFlagRequired("instance-id")(service-name 同),只靠 Run 里的手工校验:help 不标必填、报错时机也更晚,与本产品其他命令不一致。规范 §7 要求必填 flag 用 MarkFlagRequired 且描述以 Required. 开头,二者同时满足。改完按 §2.4 重刷 cmdtree.golden(required 状态入 golden)。
| cmd.Flags().SortFlags = false | ||
| ctx.BindRegion(cmd, req) | ||
| ctx.BindZone(cmd, req) | ||
| command.SetFlagValues(cmd, "region", ctx.RegionList()...) |
There was a problem hiding this comment.
🟡 这行是无效调用,请删除:BindRegion 已自动注册 region 的动态补全(规范 §8:「Region/Zone/Project 的补全由 Bind* 自动注册,产品无需手动调用任何 provider」),cobra 不允许对同一 flag 重复注册,这行被静默吞掉——completion.golden 里 describe 的 region 仍是 dynamic 可证。
| "github.com/ucloud/ucloud-cli/pkg/command" | ||
| ) | ||
|
|
||
| const StateRunning = "Running" |
There was a problem hiding this comment.
🟡 concern 文件归位(规范 §2.2/§2.3):本文件是 describe verb 文件,但塞进了三个横切关注点——
StateRunning域状态常量 →status.go(可顺手改小写,包内使用无需导出);describeClusterForPoll轮询闭包 →poll.go(describe.go 被 verb 占用时的规定去处,§2.2 命名冲突规则);sdkStr是sdk.String的重复实现(list.go 已 importsdk "github.com/ucloud/ucloud-sdk-go/ucloud"),统一用sdk.String后删除。
| } | ||
|
|
||
| // frameworkAppRow is the row for ListUHadoopFrameworkAppByUseCase output. | ||
| type frameworkAppRow struct { |
There was a problem hiding this comment.
🟡 frameworkAppRow 是死代码——list_framework_app.go 实际用的是它自己文件里定义的 frameworkRow(多一个 Versions 字段)。请删掉这个,并把 frameworkRow 挪进本文件(行结构体统一归 rows.go,规范 §2.2)。
| yes = flags.BoolP("yes", "y", false, "Do not prompt for confirmation") | ||
| async = flags.Bool("async", false, "Optional. Do not wait for upgrade to complete") | ||
| flags.StringSliceVar(&nodeNames, "node-name", nil, "Node names, required when NodeRole != master") | ||
| command.SetFlagValues(cmd, "node-role", "master", "core", "task", "client") |
There was a problem hiding this comment.
🟡 补全候选含 client,但 flag 描述(和 TEST.md)都只说 master|core|task——二者必有一错;若 API 不支持升级 client 节点,补全会诱导用户输入非法值,疑似从 add-node 复制多带了。upgrade_disk.go:60 同。修正后按 §2.4 重刷 completion.golden。
|
|
||
| # 3. 切换 Go 版本 | ||
| g use 1.25.12 | ||
| alias go=/Users/user/.g/go/bin/go |
There was a problem hiding this comment.
🟡 个人环境信息(g use 1.25.12、alias go=/Users/user/.g/go/bin/go,下文 GOROOT 同)不应入库,请改成一般性环境要求(如「Go ≥ 1.25」)。
另外文末「测试结果」表 14 项全部为 ⬜ 待测试,其中 §1.3 的两条 list 示例现状必失败(见 list.go 评论)——请在合并前真实跑完并勾选结果,这份清单是本 PR 的验收证据。
2 participants
Features:
添加uhadoop-api相关操作命令，包含以下命令：
Commands:
list List all UHadoop clusters
describe Describe a UHadoop cluster
create Create a UHadoop cluster
delete Delete a UHadoop cluster
add-node Add nodes to a UHadoop cluster
list-node-type List available node types for UHadoop
list-framework-app List UHadoop framework apps by use case
restart-service Restart/start/stop a UHadoop cluster service
upgrade-node Upgrade UHadoop node instance type
upgrade-disk Upgrade UHadoop node disk size
Fixes: